MQTT improvements and fixes #2

rechrtb · 2023-09-08T13:40:44Z

Change subscription max QOS parameter from Q to O in M586.4
Removed single default topic configuration in M586.4.
Topic to publish to must be specified in M118 via parameter T . QOS, retain and duplicate flags can be optionally specified via Q, R and D in M118
Support LWT (last will and testament) QOS and retain flag via parameter Q and R in M586.4.
MQTT demonstration script and documentation moved to Duet3D/MQTT-WPA2-Enterprise-Demo.

rechrtb · 2023-09-16T03:08:50Z

@PaulG4H, if you want to test early :) I'm testing with the setup from https://github.com/rechrtb/Duet-Demo/tree/master/mqtt, and seems to work ok when following the README, but I'm still double checking the code if I miss some potential bugs.

src/GCodes/GCodes2.cpp

src/Networking/MQTT/mqtt_pal.cpp

dc42 · 2023-09-23T14:30:38Z

src/Networking/MQTT/MqttClient.h


 	uint32_t messageTimer;	// General purpose variable for keeping track of queued messages timeout
-	Mutex publishMutex;

 	MqttClient *next;


Under what conditions can we have more than one MqttClient?

The case where more than one MqttClient instance is needed is for when there are multiple brokers - either on the same interface or on different interfaces.

Per our email conversation, only one broker on one interface needs to be supported for now. So I changed MqttClient to be a singleton. This is remnant of the old code I forgot to delete.

dc42 · 2023-09-23T14:32:32Z

src/Networking/MQTT/mqtt_pal.h

-#define MQTT_PAL_MUTEX_LOCK(x)
-#define MQTT_PAL_MUTEX_INIT(x)
-#define MQTT_PAL_MUTEX_UNLOCK(x)
+typedef struct


Please confirm that we need to use a mutex, i.e. there can be more than one task accessing those parts of the system that the mutex guards. Also, are we certain that no deadlock can occur?

The mutex is for guarding resources between (1) the main task when it calls mqtt_publish - when buffering the message for sending and (2) the network task when it calls mqtt_sync - when moving data from mqtt buffer to socket buffer.

As to whether deadlocks can occur, one of the condition for it does not seem to be satisfied - circular wait. mqtt_sync and mqtt_publish only share 1 common mutex they are both attempting to lock; and mqtt_publish itself only tries to attempt to lock 1 mutex, breaking the possibility of a circular wait chain from forming.

dc42 · 2023-09-25T13:23:54Z

Further to my previous message, I see that the current API does support starting at a specified channel when connecting to a specified SSID. See https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/network/esp_wifi.html#_CPPv417wifi_sta_config_t .

…

On Mon, 25 Sept 2023 at 08:39, rechrtb ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Networking/MQTT/MqttClient.h <#2 (comment)>: > uint32_t messageTimer; // General purpose variable for keeping track of queued messages timeout - Mutex publishMutex; MqttClient *next; The case where more than one MqttClient instance is needed is for when there are multiple brokers - either on the same interface or on different interfaces. Per our email conversation, only one broker on one interface needs to be supported for now. So I changed MqttClient to be a singleton. This is remnant of the old code I forgot to delete. — Reply to this email directly, view it on GitHub <#2 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAUYI3FIQUHMRNCHJJ3NC73X4EYLJANCNFSM6AAAAAA4QNCYPQ> . You are receiving this because you commented.Message ID: ***@***.***>

-- David Crocker, Duet3D Ltd.

PaulG4H · 2023-09-29T10:23:16Z

@rechrtb sorry for the late reply,

Many thank's for your work to bring mqtt native to RRF!

I testet the current 3.5.0-rc.1+102 Version today, it works better but the Topic in M118 gets corrupted --> when I run:
M118 P6 S"idle" T"devices/rrf-k40/status" R1

The broker receives only:

I have no clue why the topic is corrupted to only >k40<, instead of the full path >devices/rrf-k40/status<

After further testing, the topic is ignored totaly, instead the machine Name is used.

It seams that my ESP32 Firmware is not working correctly?

rechrtb · 2023-10-01T01:40:53Z

Hi @PaulG4H, no worries.

I tested sending M118 P6 S"idle" T"devices/rrf-k40/status" R1 with the setup from the demo, and seems to work ok. No topic corruption or anything like that.

I will have to ask where did you get the binary for the RRF version you tested? You might be testing one that don't have these changes yet, or at least the latest changes in this pull request. Can you try doing M118 without the T parameter? If it doesn't complain of a missing parameter, it means you're on an older version without these changes.

I'm not sure if you are able to build RRF with the changes in this pull request. If not, I can send you a build - though don't know the board you're using. Furthermore, you might be apprehensive of trusting some binary from some stranger online (rightfully so!).

@dc42 can I share with @PaulG4H a built binary with this PR using the Duet3D Dropbox?

PaulG4H · 2023-10-01T09:31:03Z

@rechrtb I already post this issue in the RRF discord --> I use a SKR2 board with the binary created by GloomyAndy and his Version is not the latest.

So I will wait for that.

I also see in the official documentation the LWT config, this is so great either!

When your changes are applied to the RRF binaries this opens a whole now world!

Smarthome Integration without pooling, no matter if you use Home Assistant, IP Symcon, IO Broker, OpenHab or any other which is able to receive MQTT topics!

Or switch a external relay from GCode (Tasmota or Shelly Device) directly over WiFi for example the Dust Collector for the CNC or the Chiller for the Laser...

Many thanks for your time and effort!

rechrtb · 2023-10-02T03:03:35Z

Unfortunately I don't have the build environment for RRF SKR setup. So if you want to test these changes, you might want to ask around if someone can do a build for you with the branch on this PR. Or, wait for this to get merged and RRF SKR project to pick up the changes like you mentioned. Great to hear about your excitement regarding this feature! Much appreciated.

…

On Sun, Oct 1, 2023, 18:31 PaulG4H ***@***.***> wrote: @rechrtb <https://github.com/rechrtb> I already post this issue in the RRF discord --> I use a SKR2 board with the binary created by GloomyAndy and his Version is not the latest. So I will wait for that. You also see in the official documentation the LWT config, this is so great! When your changes are applied to the RRF binaries this opens a whole now world! Smarthome Integration without pooling, no matter if you use Home Assistant, IP Symcon, IO Broker, OpenHab or any other which is able to receive MQTT topics! Or switch a external relay from GCode (Tasmota or Shelly Device) directly over WiFi for example the Dust Collector for the CNC or the Chiller for the Laser... Many thanks for your time and effort! — Reply to this email directly, view it on GitHub <#2 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AYN7X3RG4LDJHXJ5MFSDBPLX5EZ6FANCNFSM6AAAAAA4QNCYPQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

dc42 · 2023-10-10T12:37:08Z

@rechrtb please make that change and then merge this PR into your 3.5-dev branch; then let me know and I will merge your 3.5-dev branch into the master.

This is moved to another repo

- Fix setting connect flags related to LWT - Display debug message when changing subscription QOS - Replace max QOS argument

Since MQTT client is implemented as a singleton for now, remove code related to partially handling multiple instances.

rechrtb · 2023-10-25T07:55:10Z

Closing due to mistake in target branch. See Duet3D#923 instead.

rechrtb force-pushed the mqtt_fixes branch 6 times, most recently from e10c385 to 8e3777c Compare September 16, 2023 02:51

rechrtb changed the title ~~[WIP] MQTT fixes~~ [WIP] MQTT improvements and fixes Sep 16, 2023

rechrtb force-pushed the mqtt_fixes branch 6 times, most recently from 6b3f92e to 0900ffe Compare September 22, 2023 00:57

rechrtb changed the title ~~[WIP] MQTT improvements and fixes~~ MQTT improvements and fixes Sep 22, 2023

dc42 suggested changes Sep 23, 2023

View reviewed changes

rechrtb force-pushed the mqtt_fixes branch 2 times, most recently from a9c67a5 to 3478a74 Compare September 25, 2023 06:18

rechrtb force-pushed the mqtt_fixes branch 3 times, most recently from d4893ad to ce4a6a8 Compare September 26, 2023 12:22

rechrtb closed this Oct 1, 2023

rechrtb reopened this Oct 1, 2023

Support specifying will qos and retain

2da4fc1

rechrtb added 16 commits October 13, 2023 20:02

Use unique arg for subscribe max qos

a60e794

Implement setting topic, qos, retain and dup on M118 MQTT publish

1dfe572

Remove unneeded variable

deaa97e

Remove need for intermediate output buffer when publishing

8ec28c9

Some cleanups

00e8179

Make publish topic mandatory

f453b8c

Fix freeze when disabling MQTT

46dbdd7

Create only one mqtt client instance for now

74cfd65

Set interface null properly

07aef6b

Check if client can be enabled on multiple interfaces

24a5668

Remove mqtt demo files

ed2b130

This is moved to another repo

Fixes for MQTT client

6a9de6c

- Fix setting connect flags related to LWT - Display debug message when changing subscription QOS - Replace max QOS argument

Use GetLimitedUIValue for getting qos, retain and dup values

3278de3

Use same names for mqtt client mutex

fd624a9

Remove unecessary code for multiple mqtt clients

33ec4b2

Since MQTT client is implemented as a singleton for now, remove code related to partially handling multiple instances.

Clear send buffer error manually

3bc2d1e

rechrtb force-pushed the mqtt_fixes branch from ce4a6a8 to 3bc2d1e Compare October 13, 2023 11:02

rechrtb closed this Oct 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MQTT improvements and fixes #2

MQTT improvements and fixes #2

rechrtb commented Sep 8, 2023 •

edited

Loading

rechrtb commented Sep 16, 2023

dc42 Sep 23, 2023

rechrtb Sep 25, 2023

dc42 Sep 23, 2023

rechrtb Sep 26, 2023

dc42 commented Sep 25, 2023 via email

PaulG4H commented Sep 29, 2023 •

edited

Loading

rechrtb commented Oct 1, 2023 •

edited

Loading

PaulG4H commented Oct 1, 2023 •

edited

Loading

rechrtb commented Oct 2, 2023 via email

dc42 commented Oct 10, 2023 •

edited by rechrtb

Loading

rechrtb commented Oct 25, 2023

MQTT improvements and fixes #2

MQTT improvements and fixes #2

Conversation

rechrtb commented Sep 8, 2023 • edited Loading

rechrtb commented Sep 16, 2023

dc42 Sep 23, 2023

Choose a reason for hiding this comment

rechrtb Sep 25, 2023

Choose a reason for hiding this comment

dc42 Sep 23, 2023

Choose a reason for hiding this comment

rechrtb Sep 26, 2023

Choose a reason for hiding this comment

dc42 commented Sep 25, 2023 via email

PaulG4H commented Sep 29, 2023 • edited Loading

rechrtb commented Oct 1, 2023 • edited Loading

PaulG4H commented Oct 1, 2023 • edited Loading

rechrtb commented Oct 2, 2023 via email

dc42 commented Oct 10, 2023 • edited by rechrtb Loading

rechrtb commented Oct 25, 2023

rechrtb commented Sep 8, 2023 •

edited

Loading

PaulG4H commented Sep 29, 2023 •

edited

Loading

rechrtb commented Oct 1, 2023 •

edited

Loading

PaulG4H commented Oct 1, 2023 •

edited

Loading

dc42 commented Oct 10, 2023 •

edited by rechrtb

Loading